Migrate Ldap manager passwords to up-to-date encryption strategy by thomaslow · Pull Request #7248 · kitodo/kitodo-production

thomaslow · 2026-09-10T12:07:42Z

This PR addresses two CodeQL security warnings about outdated encryption algorithms:

"Use of a broken or risky cryptographic algorithm" in AESUtil.java:90
"Use of a broken or risky cryptographic algorithm" in AESUtil.java:155

Related Issues

Contributes to Fix security issues and warnings #6208

How it works

The class AESUtil is needed to symmetrically encrypt Ldap manager passwords. These passwords are used to write credentials of users to an Ldap server (if configured). The secret key for this encryption can be configured in kitodo_config.properties via security.secret.ldapManagerPassword.

Currently, AESUtil uses the encryption method AES/CBC, which is generally not bad, but a bit outdated. This PR migrates passwords to AES/GCM, which is considered the current standard.

This PR follows a similar strategy as #7188 and the Spring Security "DelegatingPasswordEncoder". It prefixes encrypted passwords with a unique identifier, e.g., {aes-gcm}, which is used to identify the encryption strategy for this password. Passwords that have previously stored without such a prefix are treated as "legacy" and are decrypted with the legacy decryption strategy. In the future, passwords can be easily migrated to new and better algorithms by adding another strategy implementation.

In order to migrate passwords to the newest encryption algorithm, the button LDAP Manager Passwörter migrieren on page "System -> Migrieren" can be used.

Since Kitodo.Production needs to be capable of decrypting legacy passwords for some time, the CodeQL security warning message will remain active. The legacy class AESUtil (now LegacyAesUtil) has been marked as @Deprecated.

UI Changes

The password input for ldap manager passwords was previously set up with redisplay="true", meaning, the actual clear-text password was already embedded in the HTML, but there was no "eye" button for users to display the clear-text password. This PR adds the "eye" button and fixes a simple layout issue (the password input did not stretch to 100% width).

I also added a maxlength restriction of 128 characters to the password input such that you cannot enter passwords that will be encrypted to something that does not fit the managerPassword database column that only supports 255 characters (defined as VARCHAR(255)). A password with 128 characters is encoded with roughly ~250 characters. In case the encoding still doesn't fit, a general "Ldap Server cannot be saved" error is shown to the user (as implemented in the current main branch).

Migration / Release Notes

Update encrypted LDAP Manager passwords by clicking on LDAP Manager Passwörter migrieren on page System -> Migrieren

codacy-production · 2026-09-10T12:08:46Z

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 42 complexity · 2 duplication

Metric Results

Complexity 42

Duplication 2

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer}
_{TIP This summary will be updated as you push new changes.}

+     */
+    @Test
+    public void checkIsEncrypted() throws Exception {
+        String cipher = LegacyAesUtil.encrypt(INPUT, SECRET);


+    @Test
+    public void checkIsEncrypted() throws Exception {
+        String cipher = LegacyAesUtil.encrypt(INPUT, SECRET);
+        assertTrue(LegacyAesUtil.isEncrypted(cipher));


+        assertTrue(LegacyAesUtil.isEncrypted(cipher));
+
+        String potentialCipher = "Lorem Ipsum";
+        assertFalse(LegacyAesUtil.isEncrypted(potentialCipher));


+
+        String potentialCipher = "Lorem Ipsum";
+        assertFalse(LegacyAesUtil.isEncrypted(potentialCipher));
+        assertFalse(LegacyAesUtil.isEncrypted(Base64.encode(potentialCipher)));


…gration.

… selenium tests.

stweil

@thomaslow, please address the comments from github-advanced-security. It would be good to avoid deprecated code in new contributions.

thomaslow · 2026-09-16T12:42:46Z

@stweil I am not sure why "github-advanced-security" is not smart enough to realize that these warnings concern uses of deprecated methods and classes that were marked as deprecated by this very pull request.

I marked the legacy password code as deprecated, but it is still required to migrate old password encodings to new ones. So, from now on, these deprecated classes and methods should not be used any more, but in this PR, these uses are necessary.

I can't "dismiss" these github alerts from my side (the button is greyed out), otherwise, I would have already done so.

stweil · 2026-09-16T12:54:23Z

Ah, okay. I am afraid that only @solth has the rights to dismiss the alerts from github-advanced-security.

solth · 2026-09-16T15:00:40Z

I marked the legacy password code as deprecated, but it is still required to migrate old password encodings to new ones. So, from now on, these deprecated classes and methods should not be used any more, but in this PR, these uses are necessary.

I am not sure I understand this part. If the usage of those classes and functions is still necessary (to migrate old password encodings), why mark them as "deprecated"? I understand they should not be used to encode passwords in the future, but they are still required for the migration. Is that really the definition of "deprecated"? Shouldn't they only be marked deprecated once an non-deprecated alternative exists?

thomaslow · 2026-09-16T15:37:00Z

If the usage of those classes and functions is still necessary (to migrate old password encodings), why mark them as "deprecated"?

In my opinion, @Deprecated simply is a hint for developers to "don't use this anymore", which perfectly fits this scenario.

Of course, in this PR - today - I still use this code. But I didn't add this @Deprecated warning for me or you today. The @Deprecated warning is directed towards developers that come across this code in the next 2-3 years, such that they immediately know (without an in-depth understanding of it), this code is old, should not be used any more and, potentially, can be removed soon.

Is that really the definition of "deprecated"? Shouldn't they only be marked deprecated once an non-deprecated alternative exists?

I don't think @Deprecated implies that there is some sort of replacement. I mean, in this case, there is: the new password encryption algorithm. But, generally, you can also mark features as deprecated that are scheduled to be removed without any replacements.

henning-gerhardt

I tried your changes and the following usage scenarios are tested and are working:

migrate from a plain password to latest supported algorithm
migrate from a older encrypted password to latest supported algorithm
changing password of a user
creating new user and let his password store inside LDAP

Code is clean with only a small wrong indent.

Migrate ldap manager passwords to new aes-gcm encryption strategy.

ac7a203

github-advanced-security Bot found potential problems Sep 10, 2026

View reviewed changes

thomaslow added 5 commits September 10, 2026 12:13

Fix use of incorrect secret key in EncryptionUtilsTest.

a1a75d0

Merge branch 'main' into migrate-ldap-passwords-to-secure-encryption

8f8e690

Improve code style and check IntelliJ suggestions.

11c1d7c

Improve code style and add selenium test for ldap manager password mi…

c03b92d

…gration.

Merge branch 'main' into migrate-ldap-passwords-to-secure-encryption

58e46f6

thomaslow marked this pull request as ready for review September 15, 2026 12:01

Add simple ldap manager password secret to kitodo properties file for…

c9f32a1

… selenium tests.

solth requested a review from stweil September 16, 2026 12:24

stweil suggested changes Sep 16, 2026

View reviewed changes

solth requested review from henning-gerhardt and matthias-ronge September 17, 2026 12:38

henning-gerhardt suggested changes Sep 17, 2026

View reviewed changes

Comment thread Kitodo/src/main/java/org/kitodo/production/security/encrypt/LegacyEncryptionStrategy.java Outdated

thomaslow added 2 commits September 17, 2026 16:40

Merge branch 'main' into migrate-ldap-passwords-to-secure-encryption

3ec9d17

Fix indentation in LegacyEncryptionStrategy.

6c5680a

henning-gerhardt approved these changes Sep 17, 2026

View reviewed changes

Navigation Menu

Migrate Ldap manager passwords to up-to-date encryption strategy - #7248

Migrate Ldap manager passwords to up-to-date encryption strategy#7248
thomaslow wants to merge 9 commits into
kitodo:mainfrom
thomaslow:migrate-ldap-passwords-to-secure-encryption

thomaslow commented Sep 10, 2026 •

edited

Loading

Uh oh!

codacy-production Bot commented Sep 10, 2026

Uh oh!

stweil left a comment

Uh oh!

thomaslow commented Sep 16, 2026 •

edited

Loading

Uh oh!

stweil commented Sep 16, 2026

Uh oh!

solth commented Sep 16, 2026

Uh oh!

thomaslow commented Sep 16, 2026

Uh oh!

henning-gerhardt left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

thomaslow commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issues

How it works

UI Changes

Migration / Release Notes

Uh oh!

codacy-production Bot commented Sep 10, 2026

Up to standards ✅

Uh oh!

stweil left a comment

Choose a reason for hiding this comment

Uh oh!

thomaslow commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stweil commented Sep 16, 2026

Uh oh!

solth commented Sep 16, 2026

Uh oh!

thomaslow commented Sep 16, 2026

Uh oh!

henning-gerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thomaslow commented Sep 10, 2026 •

edited

Loading

thomaslow commented Sep 16, 2026 •

edited

Loading